Check out this pull request on ReviewNB: https://app.reviewnb.com/stellargraph/stellargraph/pull/435

You'll be able to see visual diffs and write comments on notebook cells. Powered by ReviewNB.

Code Climate has analyzed commit 1dbe778 and detected 8 issues on this pull request.

Here's the issue category breakdown:

View more on Code Climate.

Notebook

stellargraph_dev/demos/interpretability/gat/node-link-importance-demo-gat.ipynb

• Jupyter notebook does not have title.
  I have added a title - Interpreting Nodes and Edges by Saliency Maps in GAT

• When you introduce importances and saliency maps you don’t seem to describe the assumptions of the functions. I believe the integrated gradient methods assume that the features are all binary. We should state this assumption clearly in the description. Also indicate what we should do if the features are not binary.

IG does seem to work well for binary features compared with vanilla methods. However, it does not assume binary features. In fact, it was initially used in the image domain where features are not binary.

• Sanity checks and others are best moved from notebooks to unit tests:

  • delta & non_exist_edge variable check
  • serialisation check
  • ego-graph integrate_link_mask check
  • masked_array check

Fixed. These sanity checks are now in the unit tests.

• Let’s do the same here as we discussed for GCN - I think the functions should all take a node ID not an index so we don’t confuse the different indices for graph vs pandas features.

• How about changing the name of get_integrated_link_masks to get_link_importance to match the get_node_importance function.
  Fixed.

• As you have used sorted_indices[::-1] in the preceding cell, in the line:

print('Top {} most important links by integrated gradients are {}'.format(topk, integrated_link_importance_rank[-topk:]))

Shouldn’t integrated_link_importance_rank[-topk:] be integrated_link_importance_rank[:topk]?

Good catch! Fixed.

• The nodes in G_ego are by IDs, therefore I think we should use target_nid not target_idx here:

  if nid == target_nid:
    continue

• In the visualisation code and elsewhere there are many instances of list(G.nodes()).index(·) that can be replaced with graph_nodes.index(·)

Fixed.

layer/graph_attention.py

• I think we need to subtract the row maximum from dense before the exponential to avoid floating point errors in the exp function:

W = (
    (1 - self.non_exist_edge) * self.delta * A
    + self.non_exist_edge
    * (
        A
        + self.delta * (K.ones(shape=[N, N], dtype=*”float”*) - A)
        + K.eye(N)
    )
) * K.exp(dense - K.max(dense, axis=1, keepdims=True))
dense = W / K.sum(W, axis=1, keepdims=True)

This means the current tests fail, but I don’t believe this is bad – the results from this are different from the implementation without the subtraction. The results in the notebook are the same with this normalisation.

There is, as you point out, an issue with the number of non-zero elements in the link importance calculation; however, I think this is just an issue of floating point accuracy, and counting elements above a small threshold works fine: i.e. using the following

print("Number of non-zero elements in integrate_link_importance: {}".format(np.sum(np.abs(integrate_link_importance) > 1e-8)))

gives:

Number of edges in the ego graph: 210
Number of non-zero elements in integrate_link_importance: 210

That's interesting. It does explain the previous test failure. Fixed in the tests.

I would like to add a unit test to the tests/layer/test_graph_attention.py file that checks that the results from the implementation with saliency_map_support=False are the same as those for saliency_map_support=True – such as the test_apply_average_with_neighbours method. Would you like to add this test?

I have changed the test to add the GAT model with saliency map support as well.

utils/saliency_maps_gat

• I think we should move these files to utils/saliency_maps and name them integrated_gradients_gat.py and saliency_gat.py . Additionally rename the IntegratedGradients class to IntegratedGradientsGAT and GradientSaliency to GradientSaliencyGAT. This way we can import all saliency objects to the same namespace in utils/saliency_maps/__init__.py.

Fixed.

• As in the comments on the notebook above, let’s use the node IDs instead of index in all functions.

Yes, fixed.

• In get_integrated_node_masks the features are taken from zero to one.

It's actually from baseline (does not necessarily be 0) to the current state of X (which does not necessarily be 1).

• This seems to assume that the features are all binary. What happens if they are not? We should state this assumption clearly in the class documentation.

Therefore, we do not assume binary features.

• This seems to only consider the importance of features that are one, as any features that are zero will be zero for all steps. I would have guessed that there would also be a function that takes those features from 1 to 0, you talked about this in the paper but I forget what you said now!

Somehow I did not implement that in GAT. Fixed now.

• There should be a description of what each method does and the assumptions made.

Fixed.

• In the argument list try not to put multiple variables on the same line, rather describe each variable separately on its own line.

Fixed.

IG does seem to work well for binary features compared with vanilla methods. However, it does not assume binary features. In fact, it was initially used in the image domain where features are not binary.

OK, thanks for clarifying! We can mention this in the class docstring, particularly the importance of setting X_baseline appropriately.

You have introduced a flag which selects if the we should set the baseline to zero and go to X_val for all features, or set the baseline to X_val and go to one for all features. I was thinking that, for binary features at least, we should set the baseline to 1-X_val then the IG can calculate the change from 1 to 0 or 0 to 1, i.e. what will happen when the feature is different. Thus when we calculate node importance, we don't have to do so twice (once for features which are 1 and then again for features which are 0). What do you think?

I tend to keep as is. Although we aimed at binary features as the motivation initially, we should not make the implementation to be specifically for that. If the features are not binary, the path from 0 -> X_val and X_val -> 1 are different, thus leading to no space for the optimization described above. Also, under the existing frameworks, calculating the gradients for part of the matrix does not really bring much performance improvements I think.

We need to think about how to return the link importance values to align with the node id rather than node indices in the parameters of saliency maps. In other words, we should not let the users do the manual re-mapping by themselves.

We need to think about how to return the link importance values to align with the node id rather than node indices in the parameters of saliency maps. In other words, we should not let the users do the manual re-mapping by themselves.

That is a good point, I didn't think about that. I think we should add this as a future issue.

I'm happy with the changes!